Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test which attempts to encounter a concurrent resize and retrieval #39

Merged
merged 7 commits into from
Jan 31, 2020

Conversation

jhinch
Copy link
Contributor

@jhinch jhinch commented Jan 29, 2020

I noticed that one of the areas of the code which had low test coverage was the BinEntry::Moved branch in node.rs. This test is designed to exercise that codepath. At least on my machine it seems to exercise the codepath but it is non-deterministic whether it will actually encounter the path.

@jhinch
Copy link
Contributor Author

jhinch commented Jan 29, 2020

Of course it decides to not hit the codepath in CI :P. How would you feel about me adding a unit test for the find method so it can be tested deterministically?

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

A unit test for that would be fantastic!

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

I also wonder whether you might be able to trigger more resizes by forcing all the keys into a single bucket like we do here:

flurry/tests/basic.rs

Lines 88 to 104 in 5fe084b

use std::hash::{BuildHasher, Hasher};
struct OneBucketState;
struct OneBucketHasher;
impl BuildHasher for OneBucketState {
type Hasher = OneBucketHasher;
fn build_hasher(&self) -> Self::Hasher {
OneBucketHasher
}
}
impl Hasher for OneBucketHasher {
fn write(&mut self, _bytes: &[u8]) {}
fn finish(&self) -> u64 {
0
}
}

This has the added benefit of avoiding code coverage fluctuating
due to non-determinism in the concurrent tests
@jhinch
Copy link
Contributor Author

jhinch commented Jan 29, 2020

At least when testing locally, using the one bucket hasher makes the BinEntry::Moved be encountered less often. I believe this is because the transfer function will transfer entire runs of entries when it can, so you actually want quite shallow bins to trigger the desired behaviour

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

This looks great! I do still wish we had a reliable way to check that the next_table pointer in Moved was set correctly, which basically requires a resize to take place while we run a find. Maybe one way to do that is to set up a very large table in the test, then call reserve with a value large enough to trigger a resize in a separate thread, and then immediately do gets on the main thread?

@jhinch
Copy link
Contributor Author

jhinch commented Jan 29, 2020

I would guess the only way to do it reliably is using a tool like loom to test each of the permutations

@jonhoo
Copy link
Owner

jonhoo commented Jan 29, 2020

I think the strategy I outlined above should actually work pretty reliably, but yes, ultimately we'll want something like loom as well (#34).

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

@jhinch would you be interested in also trying to implement the test strategy above?

Increase the resize capacity to 32768 to increase likelihood of
triggering the behaviour
@jhinch
Copy link
Contributor Author

jhinch commented Jan 30, 2020

Apologies. I thought your comments were more speculative of how things could be done rather than of something which should be changed as part of this PR. I have switched it over to use .reserve() instead and increased it to resize to 32K to increase likelihood of a resize being triggered

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

No worries. It was a bit of a speculative comment :p

I think you'll first want to do many many inserts, and then start the thread that does resizing. Otherwise the resizes will all finish very quickly, and you may not even see it.

@jhinch
Copy link
Contributor Author

jhinch commented Jan 30, 2020

It looks like I've introduced a memory leak in the unit tests I added. I'm working through the problem now.

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

Ah, yes, the tests you've written never actually free any of the memory they allocate since you do not create a Table at any point (which is the thing that actually calls defer_destroy on stuff in its bins).

@jhinch
Copy link
Contributor Author

jhinch commented Jan 31, 2020

Alright, all tests are passing again and merge conflicts have been resolved

@jonhoo jonhoo merged commit 26445cf into jonhoo:master Jan 31, 2020
@jonhoo
Copy link
Owner

jonhoo commented Jan 31, 2020

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants